Add option to flush sinks on shutdown #912
Conversation
csolidum
changed the title
csolidum
force-pushed
the
csolidum/upstream-flush
branch
from
b4dcdfb to
ace23b6
Compare
csolidum
force-pushed
the
csolidum/upstream-flush
branch
from
1c8cccb to
6311da7
Compare
|
Thanks for the change, sorry I didn't get a chance to take a look today. I'll take a look on Monday. |
server.go
Outdated
| @@ -1372,6 +1375,11 @@ func (s *Server) Shutdown() { | |||
| // TODO(aditya) shut down workers and socket readers | |||
| s.logger.Info("Shutting down server gracefully") | |||
| close(s.shutdown) | |||
| if s.FlushOnShutdown { | |||
| ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(s.Interval)) | |||
There was a problem hiding this comment.
is interval required? what happens if flush is set to true, but interval isn't set? could that potentially lead to not allowing enough time for flushing?
There was a problem hiding this comment.
It looks like veneur has a default interval setting of 10s. My concern with not setting some sort of timeout is that the sidecar might get stuck in some sort of shutdown flow. In the case of flushing taking too long, this timeout also applies to the normal flush to sinks, so if flushing takes longer than this, you're already dropping metrics.
server.go
Outdated
| @@ -1372,6 +1375,11 @@ func (s *Server) Shutdown() { | |||
| // TODO(aditya) shut down workers and socket readers | |||
| s.logger.Info("Shutting down server gracefully") | |||
| close(s.shutdown) | |||
| if s.FlushOnShutdown { | |||
| ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(s.Interval)) | |||
There was a problem hiding this comment.
Minor nit, context.WithTimeout(context.Background(), s.Interval) is one less step.
|
This change LGTM, happy to help merge after merging master into this branch. |
csolidum
force-pushed
the
csolidum/upstream-flush
branch
from
7f6349c to
570ca8a
Compare
Thanks! Rebased on the current master, feel free to merge whenever. |
Summary
Add a new config option flush_on_shutdown which when enabled, flushes sinks as part of a graceful shutdown
Motivation
This is useful for when veneur is run as a sidecar for short lived pods like cronjobs so that we can clean up resources without having to wait for an entire flush interval. Related issue is #806
Test plan
Tested a build with this locally and observed that sinks where flushed when we hit the quitquitquit endpoint.
Rollout/monitoring/revert plan
Setting the flush_on_shutdown config option as true for veneur will enable this behavior. Removing/setting this value to false will revert this.